Split CbvApplicant into three STI subclasses #451
Conversation
These subclasses contain the complexity for validating MA/NYC/Sandbox sites. The base class contains shared functionality. The underlying `cbv_applicants` table still contains all the columns for all the subclasses :-/.
tdooner
force-pushed
the
td/ffs-2456-use-key-value-table-named-wip-sti-approach
branch
from
89b41c1 to
3123eb7
Compare
Also simplify the validation logic by using Rails built-ins.
We don't need the agency prefixes anymore since Rails will automatically try to look for the superclass's validation error messages.
tdooner
requested review from
allthesignals,
GeorgeCodes19 and
jeffcatania-usds
| last_name: | ||
| blank: Enter the client's last name. | ||
| snap_application_date: | ||
| invalid_date: Enter today's date or the date you contacted the client. |
There was a problem hiding this comment.
So this would be the error that's given for agencies that are not STI? i.e. sandbox?
There was a problem hiding this comment.
Yep, it's the default used when an STI subclass doesn't override it (as cbv_applicant/ma and cbv_applicant/nyc both do.)
There was a problem hiding this comment.
Looks good to me. I'm a big fan of this approach.
I can see how things can get a bit confusing with using helper methods such as a agency_translation while having multiple language file entries
pdf:
agency_header_name:
ma: Massachusetts Department of Transitional Assistance
nyc: NYC Human Resources Administration
sandbox: CBV Test Agency
But also having the STI convention
cbv_applicant/ma:
attributes:
agency_id_number:
blank: Enter a valid agency ID number.
invalid_format: Agency ID number must be 7 digits.
beacon_id:
invalid_format: Your WELID must be 6 characters. It can only include letters and numbers.
snap_application_date:
invalid_date: Enter today's date or the date you contacted the client. This date must be today or in the past year.
cbv_applicant/nyc:
| attributes: | ||
| agency_id_number: | ||
| blank: Enter a valid agency ID number. | ||
| invalid_format: Agency ID number must be 7 digits. | ||
| beacon_id: | ||
| invalid_format: Your WELID must be 6 characters. It can only include letters and numbers. | ||
| snap_application_date: | ||
| invalid_date: Enter today's date or the date you contacted the client. This date must be today or in the past year. | ||
| cbv_applicant/nyc: |
There was a problem hiding this comment.
my MDR sense is that this is "scary," but IDk why — it's unusual. Why does it work?
There was a problem hiding this comment.
The Rails i18n gem is mysterious and important. In this case, it's because the validation error message lookup logic is aware of potential STI ancestors and tries multiple different keys until it finds one that matches.
There was a problem hiding this comment.
ahhh. i see. the inheriting model serializes its name with the slash. fascinating thx
|
r.e. @GeorgeCodes19
Yeah, this is a second pattern for have agency-specific translation overrides. However, we already had this pattern because the |
tdooner
deleted the
td/ffs-2456-use-key-value-table-named-wip-sti-approach
branch
Ticket
Related to FFS-2456. Maybe not completely solving the ultimate purpose of that ticket, but I think this will go far enough towards solving it that we can probably close that ticket out until we hit more pain from these agency-specific configurations.
Changes
Split CbvApplicant into three Single-Table Inheritance (STI) subclasses
These subclasses contain the complexity for validating MA/NYC/Sandbox
sites. The base class contains shared functionality. The underlying
cbv_applicantstable still contains all the columns for all thesubclasses :-/.
Context for reviewers
Discussed via Slack and it seems like we agree on this approach. So this is mergeable now (2/14 ❤️ ).
Acceptance testing
:alert: Deploy block! @ffs-eng I just merged PR [#123] and will be doing acceptance testing in demo - please don't deploy until I'm finished!)